[release/5.0] Support Async DNS lookup on older Windows from impersonificated conte… #47657
Conversation
…xt (dotnet#47435) * test WindowsIdentityImpersonatedTests runs * fix win8 * add test variant * final cleanup
wfurt
added
Servicing-consider
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis if follow-up on #46897 approved for 5.0 and fix for #45165. Customer ImpactAsynchronous name resolution and services depending on that (like HttpClient) do not work when running from impersonate context on Windows (ie . user other than owner of the process) This fixes variant for Windows 8 and Server 2012 Regression?Yes. This works in .NET Framework and was broken in 2.1 by dotnet/corefx#26850 when we began supporting async name resolution. As part of this we changed to use GetAddrInfoExW which has a bug in that it does not flow the impersonated token. The fix we made is to fall back to the sync API if it fails. TestingI did manual testing on Windows 8 as well I added two automated tests to cover NameResolution from impersonificated context. PerformanceThis only adds one more error code check to existing code. RiskSmall. The fix is to retry synchronous API on thread pool if async resolution fails with certain error code.
|
|
@danmosemsft do you think the above is reasonable to bring to shiproom? |
|
I would take it for a bar check . They may defer it waiting for more reports, but since it's really follow on from the last change, very low risk and it's easy to understand the change and the impact, it might go through. |
|
We added tests in the original change right -- how come they didn't break -- do we not run on 2021 or Win 8? Should we? |
|
no, tests were added in the follow-up e.g. #47435 I'm trying to port to 5.0. I was not aware that we can create ad-hoc users on CI machine as this is my first impersoniofication fix. |
leecow
added
Servicing-approved
|
@stephentoub @dotnet/ncl can you please review this pr, we need a code approval here to merge the change |
ghost
locked as resolved and limited conversation to collaborators
Fixes #45165
Technically this is continuation of #29935 which we fixed in 5.0.3 (PR #46897) and turned out to be only partial fix. This issue is specific to Win8 and Win 2012 (as reported in #45165).
The original issue #29935 didn't have test (because we didn't know how), but we came up with idea later. The test discovered the same problem on Win8 and Win 2012 as the reported issue #45165.
Customer Impact
Asynchronous name resolution and services depending on that (like HttpClient) do not work when running from impersonate context on Win8 and Win 2012 (ie . user other than owner of the process)
At least 1 customer hit this on Win 2012 in #45165.
The issue has been present for some time, but is now blocking migration from Framework to .NET Core.
Regression?
Yes. This works in .NET Framework and was broken in .NET Core 2.1 by dotnet/corefx#26850 when we began supporting async name resolution. As part of this we changed to use GetAddrInfoExW which has a bug in that it does not flow the impersonated token.
We worked around the OS bug in #29935, but we missed the case for Win8 and Win 2012, which we are fixing in this PR.
Testing
Added 2 automated tests to cover NameResolution from impersonificated context.
Performance
This only adds one more error code check to existing code. No neccesary.
Risk
Small. The fix is to retry synchronous API on thread pool if async resolution fails with certain error codes.
The logic already exists, we are just using it for more OS error codes.